Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIDY FIRST] Type BaseRunner class #10607

Closed
wants to merge 17 commits into from
Closed

Conversation

QMalcolm
Copy link
Contributor

@QMalcolm QMalcolm commented Aug 26, 2024

Resolves #10606

Problem

The BaseRunner class is largely untyped. Given the prevalence of this class, not having it typed could mean that there are potential edge cases and/or runtime exceptions we are not aware of. By typing this class, we give ourselves a lot more guarantees

Solution

Add typing. Of note, adding this typing uncovered a potential runtime exception in the on_skip method. We address this in 5adce38.

Checklist

  • I have read the contributing guide and understand what's expected of me.
  • I have run this code in development, and it appears to resolve the stated issue.
  • This PR includes tests, or tests are not required or relevant for this PR.
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
  • This PR includes type annotations for new and modified functions.

The `BaseRunner` class is a widely inherited class, and it's untyped.
Sometimes the wrong thing gets passed into the classes that inherit
`BaseRunner`. The "subclasses" of `BaseRunner` generally further
restrict some of the available typing, (f.x. the type of node that
can be passed in). In order to properly type those subclasses, we
first should type `BaseRunner`. This is the start of that.
As part of this I had to switch the typing of node from `GraphMemberNode`
to `ManifestNode`. We needed to do this be because `GraphMemberNode` was
_too_ wide. Specifically, `GraphMemberNode` includes the `Metric` node
which doesn't inherit from `NodeInfoMixin` and thus does not have a
`update_event_status` method. Unfortunately moving to `ManifestNode`
_excludes_ some nodes from the allowed typing that we'll probably later
on find we need to include. Maybe that means some nodes should be moved
to the `ManifestNode` definition. We might find that it would be more
appropriate however to add an `ExecutableNode` or `RunnableNode` protocol
definition, and then use that for typing.
@QMalcolm QMalcolm requested a review from a team as a code owner August 26, 2024 19:20
@cla-bot cla-bot bot added the cla:yes label Aug 26, 2024
@QMalcolm QMalcolm force-pushed the qmalcolm--typing-base-runner branch from 11abfa8 to fb6c1ba Compare August 26, 2024 19:21
@QMalcolm QMalcolm added the artifact_minor_upgrade To bypass the CI check by confirming that the change is not breaking label Aug 26, 2024
…_result` of `BaseRunner`

There are two type errors in this commit. The type hinting of `RunResult.node` and the `node`
argument of `_build_run_result` are _different_! I tried to fix this in a previous commit
by going from `GraphMemberNode` too `ManifestNode` as `GraphMemberNode` was too broad. It seems
`ManifestNode` is too narrow unfortunately. Checking the typing of `RunResult.node`, which I
should have done sooner, it uses `ResultNode`, which makes sense.

The second type error is that the type hinting of `RunResult.status` and the `status` arguement
of `_build_run_result` are _different_. The correct typing we should be that of `RunResult.status`.
Both of these typing issues will be resolved in the next commit.
We did this because in `BaseRunner.compile_and_execute` we weren't
getting type completion for accessing `ctx.node.node_info`. Said another
way, we didn't have the type context in `compile_and_execute` to know
if a `node_info` property existed on the `node` of the `ctx`. By improving
type hinting for `ExecutionContext` we were able to resolve this.
Because `execute` didn't have it's return type specified, it appeared to
`run` that there was no expected return value. In turn `compile_and_run`
was saying that the `result` it would return would _always_ be `None`. However
that is most assuredly _not_ the case, and typing `execute` solves this.
Specifically methods:
- `_handle_catchable_exception`
- `_handle_internal_exception`
- `_handle_generic_exception`
- `handle_exception`

Additionally we are defining "catchable errors" twice now. Once as a union of
types in `_handle_catchable_exception` and as a tuple of classes in `handle_exception`.
We should look at refactoring this into a single definition sometime, as a change to
one should 1-1 correspond to a change in the other.
These two methods are essentially just additional logging based for the
runner. They should never return anything. Typing them as such helps
guarantee that so people don't end up doing some weird things.
In adding type hinting to `on_skip`, and `do_skip`, a potential runtime
error was uncovered wherein we were accessing `self.skip_cause.status` in
`on_skip` when the `skip_cause` can be `None`. On that edge case, a
runtime error would be raised given how we were attempting to access
the `status` property. We now handle this edge case, and we only discovered
it because of the additional typing.
@QMalcolm QMalcolm force-pushed the qmalcolm--typing-base-runner branch from fb6c1ba to 3b2481b Compare August 26, 2024 19:34
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.86%. Comparing base (bba020f) to head (f500372).
Report is 112 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10607      +/-   ##
==========================================
- Coverage   88.91%   88.86%   -0.05%     
==========================================
  Files         180      180              
  Lines       22755    22767      +12     
==========================================
+ Hits        20232    20233       +1     
- Misses       2523     2534      +11     
Flag Coverage Δ
integration 86.11% <92.50%> (-0.11%) ⬇️
unit 62.34% <90.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Unit Tests 62.34% <90.00%> (-0.01%) ⬇️
Integration Tests 86.11% <92.50%> (-0.11%) ⬇️

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
@MichelleArk MichelleArk added the tidy_first "Tidy First" incremental cleanup changes label Sep 3, 2024
@QMalcolm
Copy link
Contributor Author

QMalcolm commented Nov 8, 2024

The BaseRunner and it's derivatives unfortunately need to be refactored before we can properly type them 😞

@QMalcolm QMalcolm closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
artifact_minor_upgrade To bypass the CI check by confirming that the change is not breaking cla:yes tidy_first "Tidy First" incremental cleanup changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TIDY FIRST] Add typing to BaseRunner class
3 participants